GH-49826: [Python] Scalar arithmetic dunders return NotImplemented on unknown operand types#49833
GH-49826: [Python] Scalar arithmetic dunders return NotImplemented on unknown operand types#49833SAY-5 wants to merge 2 commits intoapache:mainfrom
Conversation
…ted on unknown operand types pyarrow 24.0.0 added Scalar.__add__/__sub__/__mul__/__truediv__/__pow__ (and bitwise ops) that unconditionally call pc.call_function. For operand types compute doesn't recognize, call_function raises TypeError -- Python's data model only triggers reflected-operator fallback when the forward operator returns NotImplemented, not when it raises. Any user class that used to handle `pyarrow.Scalar + obj` via __radd__ was therefore broken (apacheGH-49826). Route every binary dunder through a small _binop_or_notimplemented helper that catches the TypeError from _pack_compute_args and returns NotImplemented instead. Unary __neg__/__abs__ are unaffected. Closes apacheGH-49826. Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
|
|
AlenkaF
left a comment
AlenkaF
left a comment
There was a problem hiding this comment.
Thanks for the PR!
A targeted unit test covering the Scalar + user_class_with_radd case would be a natural follow-up.
I think a test similar to the repr in the connected issue would be needed as part of this PR.
Also, I would love to see the PR extend to cover array.pxi as well. Is this feasible?
| except TypeError: | ||
| return NotImplemented |
There was a problem hiding this comment.
Could we change this in a way that errors coming from call_function would not end up becoming NotImplemented?
github-actions
Bot
added
awaiting committer review
| except TypeError: | ||
| return NotImplemented |
There was a problem hiding this comment.
Agree with @AlenkaF here, the exception should be properly raised and chained:
except TypeError as e:
raise NotImplemented from e
…add tests Per @AlenkaF review on apacheGH-49833.
|
Pushed d6b1b82: extended the same NotImplemented fallback to Array.add/sub/mul/truediv/pow/and/or/xor/lshift/rshift via _array_binop_or_notimplemented, plus radd tests in both test_scalars.py and test_array.py mirroring the linked issue's repr. |
|
Both review items are addressed in d6b1b82:
Happy to add a similar test on the |
3 participants
Rationale for this change
pyarrow.lib.Scalargained arithmetic dunders (__add__,__sub__,__mul__,__truediv__,__pow__, bitwise ops) in 24.0.0. They unconditionally callpyarrow.compute.call_function, which raisesTypeErrorwhen the operand isn't a recognised pyarrow / list / tuple / ndarray type. Per the Python data model, a forward operator must returnNotImplemented(not raise) for Python to attempt the reflected operator. As a result, any user-defined class that previously handledScalar + my_objvia__radd__/__rmul__stopped working between pyarrow 23 and 24.What changes are included in this PR?
Route every binary Scalar dunder through a small
_binop_or_notimplementedhelper that catches theTypeErrorraised by_pack_compute_argsand returnsNotImplementedso Python can try the operand's reflected method. Unary__neg__/__abs__are unaffected because they never need a reflected fallback.Are these changes tested?
Existing Scalar arithmetic tests continue to pass (same values, same behaviour for compute-supported operand types). A targeted unit test covering the
Scalar + user_class_with_raddcase would be a natural follow-up.Are there any user-facing changes?
Yes, and it restores the 23.x behaviour:
Scalar.__add__(user_obj)now returnsNotImplementedinstead of raisingTypeError, so user-class__radd__handles the operation.Closes #49826.
Signed-off-by: SAY-5 SAY-5@users.noreply.github.com